Fixed some bugs and added QoL features #181

MissingReports · 2024-07-29T17:51:36Z

Changes

Made stealable items moveable regardless whether they were in a section or no
Why: If someone specifically marked a slot as stealable, i see NO reason why this should still be cancelling the event. To move sectioned slots the slot itself has to be stealable, if the gui is stealable but the slot isn't then it won't let you move it
Made a new Slot Changed section, which is triggered when a specific slot was changed
Why: This makes it easier to detect changes. Before, it wasn't even possible to detect when you instantly move an item from your inventory into the GUI. Most of the time, cancelling the event won't do anything in a Slot Changed section. You might think this is useless but it has (for me at least) alot of usecases, such as custom anvil or custom crafting table
Some changes to the code
Why: Some parts were causing a warning because they weren't properly coded, such as if List#size == 0 instead of if List#isEmpty

For the most part, I added stuff to skript-gui. It won't affect others and would be useful in some situations. So I hope this PR get's accepted or added in some way

Fixed some bugs regarding removeable/stealable GUI items

APickledWalrus · 2024-08-14T23:55:42Z

I'll take a look into reviewing this soon, sorry for the delay

APickledWalrus

Thanks for the contribution! Here are some of the things that I noticed. Please feel free to reply to any of my comments if you have any questions/thoughts 🙂

APickledWalrus · 2024-08-22T16:48:44Z

src/main/java/io/github/apickledwalrus/skriptgui/elements/sections/SecMakeGUI.java

@@ -167,4 +167,7 @@ public String toString(@Nullable Event e, boolean debug) {
 		}
 	}

+	public Expression<Object> getSlots() {


Is this used anywhere? If not, it should be removed.

I think I accidently auto generated this part

APickledWalrus · 2024-08-22T16:54:50Z

src/main/java/io/github/apickledwalrus/skriptgui/gui/events/GUIEvents.java

-							event.setCancelled(true);
-							break;
+					ItemStack cursor = event.getCursor();
+					if (cursor != null) {


This code is starting to go off into space a little bit. It would be better to terminate exit early when possible instead. That is, do something like:

if (cursor == null) return;

APickledWalrus · 2024-08-22T17:01:44Z

src/main/java/io/github/apickledwalrus/skriptgui/elements/sections/SecSlotChange.java

+		Integer[] slots = guiSlots.getAll(e);
+
+		for (Integer slot : slots) {
+			if (gui != null && slot != null && slot >= 0 && slot + 1 <= gui.getInventory().getSize()) {


the gui != null check should be moved up:

if (gui == null) return walk(e, false);

additionally, slot won't ever be null

src/main/java/io/github/apickledwalrus/skriptgui/gui/GUIEventHandler.java

APickledWalrus · 2024-08-22T17:31:56Z

src/main/java/io/github/apickledwalrus/skriptgui/gui/events/GUIEvents.java

 										continue;
 									}
 									event.setCancelled(true);
+
+									eventHandler.onChange(clickEvent);


I don't think it's appropriate to call this here. The slot isn't removable.

APickledWalrus · 2024-08-22T17:41:16Z

src/main/java/io/github/apickledwalrus/skriptgui/gui/events/GUIEvents.java

-							return;
+						if (firstEmpty != -1) { // Safe to be moved into the GUI
+							InventoryClickEvent clickEvent = gui.setClickedSlot(event, firstEmpty);
+							eventHandler.onChange(clickEvent);


This should only be called if it is safe to move something into the slot I think

That part was causing a bug, I don't remember it tho

Would you be able to test it again? My concern here is if the slot is locked but doesn't have anything in it.

I would recommend removing this (per our discussion above) and let me know if it gives you any issues (include code if applicable please 🙂)

I would recommend removing this (per our discussion above) and let me know if it gives you any issues (include code if applicable please 🙂)

If this part is removed, shift clicking an item from your inventory into an empty slot in the gui won't trigger the on change

APickledWalrus · 2024-08-22T18:31:38Z

src/main/java/io/github/apickledwalrus/skriptgui/gui/GUI.java

@@ -233,6 +265,16 @@ public Character convert(Object slot) {
 		return nextSlot();
 	}

+	public InventoryClickEvent setClickedSlot(InventoryClickEvent event, int slot) {


This would be better off as a static method in GUIEvents

APickledWalrus · 2024-08-22T18:38:43Z

src/main/java/io/github/apickledwalrus/skriptgui/gui/events/GUIEvents.java

+								if (cursor.getAmount() < cursor.getMaxStackSize()) {
+									InventoryClickEvent clickEvent = gui.setClickedSlot(event, slot);
+									eventHandler.onChange(clickEvent);
+								}


I don't think this is accurate enough. You need to keep a running total of how many items have merged into the cursor. Once you hit the max stack size the other slots (even if they have the same item) won't be modified. Same applies for the below case.

APickledWalrus · 2024-08-22T18:40:50Z

src/main/java/io/github/apickledwalrus/skriptgui/gui/GUI.java

+				e.setCancelled(!isRemovable(slotData));
+
+				Consumer<InventoryClickEvent> runOnChange = slotData.getRunOnChange();
+				if (runOnChange != null) {


You shouldn't run the consumer if the event is cancelled.

… and the opposite Removed unwanted code

MissingReports · 2024-08-23T18:09:49Z

Thanks for the contribution! Here are some of the things that I noticed. Please feel free to reply to any of my comments if you have any questions/thoughts 🙂

I have committed the updated code, fixing all the previous mistakes

src/main/java/io/github/apickledwalrus/skriptgui/elements/sections/SecMakeGUI.java

src/main/java/io/github/apickledwalrus/skriptgui/gui/events/GUIEvents.java

src/main/java/io/github/apickledwalrus/skriptgui/gui/GUI.java

…ions/SecMakeGUI.java Co-authored-by: Patrick Miller <[email protected]>

…IEvents.java Co-authored-by: Patrick Miller <[email protected]>

APickledWalrus

Mainly just the one change that needs further testing. Other than that, this should be good to go. Thank you for your work!!

src/main/java/io/github/apickledwalrus/skriptgui/gui/events/GUIEvents.java

APickledWalrus · 2024-08-26T22:13:47Z

src/main/java/io/github/apickledwalrus/skriptgui/gui/events/GUIEvents.java

-							return;
+						if (firstEmpty != -1) { // Safe to be moved into the GUI
+							InventoryClickEvent clickEvent = gui.setClickedSlot(event, firstEmpty);
+							eventHandler.onChange(clickEvent);


I would recommend removing this (per our discussion above) and let me know if it gives you any issues (include code if applicable please 🙂)

…IEvents.java Co-authored-by: Patrick Miller <[email protected]>

Added slot changed section

04e0807

Fixed some bugs regarding removeable/stealable GUI items

APickledWalrus changed the base branch from master to dev/1.4.0 August 22, 2024 16:47

APickledWalrus added the enhancement New feature or request label Aug 22, 2024

APickledWalrus requested changes Aug 22, 2024

View reviewed changes

Fixed some edge cases to change the GUI without calling the on change…

e54f10b

… and the opposite Removed unwanted code

APickledWalrus requested changes Aug 23, 2024

View reviewed changes

MissingReports and others added 3 commits August 25, 2024 16:58

Update src/main/java/io/github/apickledwalrus/skriptgui/elements/sect…

7550096

…ions/SecMakeGUI.java Co-authored-by: Patrick Miller <[email protected]>

Update src/main/java/io/github/apickledwalrus/skriptgui/gui/events/GU…

5c14eef

…IEvents.java Co-authored-by: Patrick Miller <[email protected]>

Changed the code abit

b1fb0d8

APickledWalrus requested changes Aug 26, 2024

View reviewed changes

MissingReports and others added 3 commits September 5, 2024 04:18

Update src/main/java/io/github/apickledwalrus/skriptgui/gui/events/GU…

eddc9ae

…IEvents.java Co-authored-by: Patrick Miller <[email protected]>

Patched a bug to trigger on change without changing the slot

1e7373e

Merge remote-tracking branch 'origin/master'

44b4f5c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed some bugs and added QoL features #181

Fixed some bugs and added QoL features #181

MissingReports commented Jul 29, 2024

APickledWalrus commented Aug 14, 2024

APickledWalrus left a comment

APickledWalrus Aug 22, 2024

MissingReports Aug 23, 2024

APickledWalrus Aug 22, 2024

APickledWalrus Aug 22, 2024

APickledWalrus Aug 22, 2024

APickledWalrus Aug 22, 2024

MissingReports Aug 23, 2024

APickledWalrus Aug 23, 2024

MissingReports Aug 23, 2024

APickledWalrus Aug 26, 2024 •

edited

Loading

MissingReports Sep 5, 2024

APickledWalrus Aug 22, 2024

APickledWalrus Aug 22, 2024

APickledWalrus Aug 22, 2024

MissingReports commented Aug 23, 2024

APickledWalrus left a comment

APickledWalrus Aug 26, 2024 •

edited

Loading

Fixed some bugs and added QoL features #181

Are you sure you want to change the base?

Fixed some bugs and added QoL features #181

Conversation

MissingReports commented Jul 29, 2024

Changes

APickledWalrus commented Aug 14, 2024

APickledWalrus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

APickledWalrus Aug 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MissingReports commented Aug 23, 2024

APickledWalrus left a comment

Choose a reason for hiding this comment

APickledWalrus Aug 26, 2024 • edited Loading

Choose a reason for hiding this comment

APickledWalrus Aug 26, 2024 •

edited

Loading

APickledWalrus Aug 26, 2024 •

edited

Loading